Skip to content

build(cmake): only allow to build as python extension - #152

Merged
robertodr merged 53 commits into
mainfrom
chore-dev-single-build-path
Aug 3, 2026
Merged

build(cmake): only allow to build as python extension#152
robertodr merged 53 commits into
mainfrom
chore-dev-single-build-path

Conversation

@robertodr

@robertodr robertodr commented Jul 24, 2026

Copy link
Copy Markdown
Member

By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.

Summary

This project is purely a Python extension but the build system, and configuration around it, still offered the (vestigial) option of building as a C++-only project.

Re-hash of #106, where I made a hideous mistake with the git history.

Fixes #136

Changes

  • Remove CMake code that allowed to build as a standalone C++ project, as that is no longer meaningful. A warning is issued against compiling with direct invocation to CMake.

  • C++ unit tests are gated behind a new CMake option monoprop_ENABLE_CXX_UNIT_TESTS, which is ON by default.

  • CMakePresets.json changed accordingly, as it's effectively only needed for test autodiscovery.

  • Edit justfile to account for loss of many presets for CMake.

  • Overhaul CMake code for installation of artifacts. The package is installed under lib/python3.X/site-packages/monoprop all other artifacts are installed in subfolders of it:

    • libmonoprop.so is under lib (or lib64, depending on the machine)
    • all headers are under include with the proper hierarchical folder structure
    • CMake helper files are under cmake The header files are now explicitly listed (instead of globbed) and we use target_sources with FILE_SET pento handle them sanely (instead of appending to a global list)

    I have dropped the distinction between private and public for now.

  • Simplified test.yml accordingly:

    • we no longer need separate jobs to run the C++ unit tests: they can be compiled within a wheel build and run, if need be.
    • Single coverage job running both Python and C++ tests.

Checklist

  • Tests added or updated to cover the changes
  • Documentation updated (docstrings, docs/, CONTRIBUTING.md) if needed
  • CHANGELOG / release notes updated if applicable

AI/LLM disclosure

  • I did not use LLM tooling, or used it only privately for ideation
  • I used the following tool to help write this PR description:
  • I used the following tool to generate or modify code: GitHub Copilot with auto-selection of model.

This is part 1 of 3 in a stack made with GitButler:

@github-actions

Copy link
Copy Markdown

Docs preview: https://pr-152.monoprop-docs.pages.dev

@robertodr
robertodr force-pushed the chore-dev-single-build-path branch 2 times, most recently from a3a3686 to a8aa7c6 Compare July 26, 2026 08:55
@robertodr robertodr changed the title build: only allow to build as python extension build(cmake): only allow to build as python extension Jul 26, 2026
Comment thread src/monoprop/bindings/CMakeLists.txt Outdated
@robertodr
robertodr force-pushed the chore-dev-single-build-path branch from 4e6f1e6 to afd0488 Compare July 26, 2026 09:37
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.31%. Comparing base (538ed84) to head (9885641).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   87.61%   95.31%   +7.69%     
==========================================
  Files          59       14      -45     
  Lines        4239      704    -3535     
  Branches     1497       88    -1409     
==========================================
- Hits         3714      671    -3043     
+ Misses        163       20     -143     
+ Partials      362       13     -349     
Flag Coverage Δ
cpp 95.31% <ø> (+9.23%) ⬆️
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@robertodr
robertodr force-pushed the chore-dev-single-build-path branch 3 times, most recently from ddc660a to d9fd7a3 Compare July 29, 2026 11:07
Comment thread .devcontainer/postStartCommand.sh
Comment thread .github/workflows/test.yml Outdated
@robertodr

Copy link
Copy Markdown
Member Author

The PR is still in progress, it's not marked as draft as I need all CI jobs to run on each commit to check I'm not losing functionality. Sorry for the confusion!

@robertodr
robertodr force-pushed the chore-dev-single-build-path branch 4 times, most recently from 0485838 to dad84db Compare July 29, 2026 18:59
@robertodr

Copy link
Copy Markdown
Member Author

this will also need to be merged by bypassing requirements, since the job names changed. Plus a change in required checks in the ruleset.

@robertodr
robertodr requested review from Copilot and ludmilaasb July 30, 2026 11:05
@robertodr

Copy link
Copy Markdown
Member Author

This is now ready-ready to review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the build/CI tooling to treat monoprop as a scikit-build-core–driven Python extension only, removing the remaining “standalone CMake project” pathways, and reshaping install + test/coverage flows around the editable build trees that uv sync creates.

Changes:

  • Make top-level CMake configuration scikit-build-core–centric (with direct-CMake invocation discouraged) and gate C++ unit tests behind a dedicated option.
  • Rework CMake install and header handling to use target_sources(FILE_SET ...), installing artifacts under the Python package layout.
  • Simplify developer/CI workflows (presets, VS Code tasks, just recipes, GitHub Actions) to build/test via the scikit-build-core editable trees and consolidate coverage.

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
uv.lock Removes gcovr and its transitive lock entries from the locked environment.
pyproject.toml Drops gcovr from dev deps; updates cibuildwheel env to explicitly disable C++ unit tests during wheel builds.
CMakeLists.txt Moves to scikit-build-core–first configuration, adds monoprop_ENABLE_CXX_UNIT_TESTS, and gates tests/cpp accordingly.
src/CMakeLists.txt Updates CMake package config install location and version-file generation for the new install layout.
src/monoprop/CMakeLists.txt Switches to FILE_SET-based header installation, adjusts install destinations under the Python package, and reorganizes subdirectories.
src/monoprop/bindings/CMakeLists.txt Removes local Python discovery, updates compile flag handling, and rewrites RPATH logic for the new package layout.
src/monoprop/algebra/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/core/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/CMakeLists.txt Adds explicit header listing via FILE_SET and enumerates detail subdirectories.
src/monoprop/detail/evolution/CMakeLists.txt Adds explicit header listing via FILE_SET and includes layer_build.
src/monoprop/detail/evolution/layer_build/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/graph/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/graph_encoding/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/monomial_propagator/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/mpi/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/operator/CMakeLists.txt Adds explicit header listing via FILE_SET.
src/monoprop/detail/pare/CMakeLists.txt Adds explicit header listing and adds PareGraph.cpp as a private source.
src/monoprop/detail/partition/CMakeLists.txt Adds explicit header listing via FILE_SET.
tests/cpp/CMakeLists.txt Ensures CPM is included and configures msgpack-cxx via CPM for the C++ test suite.
tests/cpp/boostAddTests.cmake Labels C++ tests with cxx for CTest filtering/reporting.
justfile Consolidates test recipes and reworks the coverage flow to run via scikit-build-core editable trees.
CMakePresets.json Replaces standalone configure/build/test presets with “adopt existing skbuild tree” presets.
.vscode/tasks.json Switches VS Code tasks to uv sync-based install modes (Release/Debug/Coverage).
.vscode/settings.json Disables CMake Tools auto-reconfigure and enables CTest Explorer integration for adopted build trees.
.github/workflows/test.yml Overhauls CI into package-centric matrix runs, adds find_package(monoprop) smoke test, runs C++ unit tests via ctest, and consolidates coverage jobs.
.github/workflows/docpages.yml Sets env to disable C++ unit tests for docs builds.
.github/workflows/copilot-setup-steps.yml Sets CPM_USE_LOCAL_PACKAGES and updates uv sync invocation for setup.
.devcontainer/postStartCommand.sh Makes prek install overwrite existing hooks in the devcontainer.

Comment thread CMakeLists.txt
Comment thread src/CMakeLists.txt
Comment thread src/monoprop/bindings/CMakeLists.txt Outdated
Comment thread justfile Outdated
Comment thread .github/workflows/test.yml Outdated
@robertodr
robertodr force-pushed the chore-dev-single-build-path branch 3 times, most recently from ae25e7c to 2587fab Compare July 30, 2026 13:25
@robertodr
robertodr force-pushed the chore-dev-single-build-path branch from 88dfbf7 to 72965c2 Compare August 2, 2026 14:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

.github/workflows/test.yml:209

  • The job expects a scikit-build-core Coverage build tree (build/editable/Coverage) later (gcovr + ctest), but the install step runs uv sync without selecting Coverage build type. Unless an undocumented env var happens to be honored, build/editable/Coverage won’t exist and the subsequent steps will fail.
      - name: Install package
        run: |
          uv sync --no-progress --group test --all-extras -v

.github/workflows/test.yml:266

  • SKBUILD_CMAKE_DEFINE is not the documented mechanism for passing CMake definitions to scikit-build-core in this repo; use SKBUILD_CMAKE_ARGS (or pass --config-settings=cmake.define... to uv sync) so MPI is reliably enabled for Sonar’s compilation database.
    env:
      SKBUILD_CMAKE_DEFINE: "monoprop_ENABLE_MPI=ON"

.github/workflows/test.yml:176

  • Codecov’s files: input is typically parsed as a comma-separated list; the second entry here has a leading space, which can be interpreted as part of the filename and cause the ctest report upload to be skipped.
      - name: Upload test results to Codecov
        uses: codecov/codecov-action@v7.0.0
        with:
          token: ${{ secrets.CODECOV_TOKEN }}
          files: pytest-${{ steps.test-group-label.outputs.label }}.xml, ctest-${{ steps.test-group-label.outputs.label }}.xml
          report_type: test_results

CMakeLists.txt:8

  • This project now relies on scikit-build-core-only configuration, but the current direct-CMake path only emits a warning and then continues into SKBUILD-specific code paths (which can fail later with less actionable errors). If direct CMake invocation is intentionally unsupported, prefer failing fast here.
if(NOT SKBUILD)
  message(
    WARNING
    "\

.github/workflows/test.yml:60

  • SKBUILD_CMAKE_DEFINE is not the documented knob for scikit-build-core in this repo (docs use SKBUILD_CMAKE_ARGS / --config-settings=cmake.define...). If this env var is ignored, the matrix ‘wide term index’ build won’t actually be wide.

This issue also appears in the following locations of the same file:

  • line 171
  • line 206
  • line 265
    env:
      SKBUILD_CMAKE_DEFINE: "monoprop_WIDE_TERM_INDEX=${{ matrix.wide }}"

.github/workflows/docpages.yml:44

  • SKBUILD_CMAKE_DEFINE is not the documented knob for scikit-build-core in this repo. If it’s ignored, the docs build will still compile the C++ unit tests even though this workflow intends to disable them.
    env:
      SKBUILD_CMAKE_DEFINE: monoprop_ENABLE_CXX_UNIT_TESTS=OFF

justfile:49

  • The repo docs still reference the old Just recipes (test-py, test-py-mpi, test-py-mpi-matrix) that were removed/renamed here (see docs/content/docs/testing.mdx:18,26-27). This makes the documented test commands incorrect after this change.
test:
    uv run python -m pytest -m "not mpi"
    ctest --test-dir build/editable/Release --output-on-failure

# MPI is off by default in source builds, so build an MPI-enabled editable install
# first, then run the suite under mpiexec with --no-sync (avoids a per-rank resync).
# Pass RANKS as either a single integer or a semicolon-separated list (e.g. "1;2;4").

test-mpi RANKS='':
    export OMPI_MCA_rmaps_base_oversubscribe="1"
    uv sync --all-extras --group test --reinstall-package monoprop --no-cache --config-settings-package="monoprop:cmake.define.monoprop_ENABLE_MPI=ON" -v
    ranks="${1:-${monoprop_MPI_TEST_PROCS:-2}}"
    for r in ${ranks//;/ }; \
    do echo "Running full Python test suite with ${r} MPI rank(s)"; \
    mpiexec -n "$r" uv run --no-sync python -m pytest tests --with-mpi -v; \
    echo "Running C++ unit tests with ${r} MPI rank(s)"; \
    ctest --test-dir build/editable/Release --output-on-failure \
    done

CMakeLists.txt:32

  • include/monoprop/Info.h is generated from include/monoprop/Info.h.in, but the placeholders @PROJECT_VERSION_FULL@ and @GIT_COMMIT@ are no longer defined anywhere in CMake (DynamicVersion was removed). As a result, monoprop::version() and monoprop::git_commit() will expand to empty strings in the built artifact.
endif()

if(APPLE)

Comment thread justfile
Comment thread justfile Outdated
Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
@robertodr

Copy link
Copy Markdown
Member Author

Comments to address from Copilot:

justfile:49

  • The repo docs still reference the old Just recipes (test-py, test-py-mpi, test-py-mpi-matrix) that were removed/renamed here (see docs/content/docs/testing.mdx:18,26-27). This makes the documented test commands incorrect after this change.
test:
    uv run python -m pytest -m "not mpi"
    ctest --test-dir build/editable/Release --output-on-failure

# MPI is off by default in source builds, so build an MPI-enabled editable install
# first, then run the suite under mpiexec with --no-sync (avoids a per-rank resync).
# Pass RANKS as either a single integer or a semicolon-separated list (e.g. "1;2;4").

test-mpi RANKS='':
    export OMPI_MCA_rmaps_base_oversubscribe="1"
    uv sync --all-extras --group test --reinstall-package monoprop --no-cache --config-settings-package="monoprop:cmake.define.monoprop_ENABLE_MPI=ON" -v
    ranks="${1:-${monoprop_MPI_TEST_PROCS:-2}}"
    for r in ${ranks//;/ }; \
    do echo "Running full Python test suite with ${r} MPI rank(s)"; \
    mpiexec -n "$r" uv run --no-sync python -m pytest tests --with-mpi -v; \
    echo "Running C++ unit tests with ${r} MPI rank(s)"; \
    ctest --test-dir build/editable/Release --output-on-failure \
    done

CMakeLists.txt:32

  • include/monoprop/Info.h is generated from include/monoprop/Info.h.in, but the placeholders @PROJECT_VERSION_FULL@ and @GIT_COMMIT@ are no longer defined anywhere in CMake (DynamicVersion was removed). As a result, monoprop::version() and monoprop::git_commit() will expand to empty strings in the built artifact.
endif()

if(APPLE)

They were only needed when building as a standalone C++ project. The same information can now be obtained from the Python layer.
@sonarqubecloud

sonarqubecloud Bot commented Aug 3, 2026

Copy link
Copy Markdown

@robertodr
robertodr merged commit fc1b9dc into main Aug 3, 2026
28 checks passed
@robertodr
robertodr deleted the chore-dev-single-build-path branch August 3, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refine coverage collection for C++ files

3 participants